Add spatial unit classes #928
Conversation
…lasses for versioned-cell and versioned-site
…sponding changes to AdminSpatialUnit and GridSpatialUnit
…ectories and ModalLocation
|
|
||
| Parameters | ||
| ---------- | ||
| criterion : str |
|
Man, and I thought I had a tendency towards giant PRs ;) I think the overall thrust of this is good (and it is a marathon effort), more once I've looked in detail. |
| from marshmallow.validate import OneOf | ||
|
|
||
|
|
||
| class AggregationUnit(String): |
There was a problem hiding this comment.
(Maybe not now, but) this is a prime target for shifting to a JSON object which maps to the spatial objects.
There was a problem hiding this comment.
Yes, I agree. I haven't attempted to implement this shift, partly because I've attempted to avoid changing any behaviour in this PR and partly because I think this change would be best as part of a wider consideration of how we want to expose aggregation units (and the associated permissions). But anticipation of this change was one of the reasons for moving this definition out of custom_fields.py.
|
With the GeoDataMixin change - can we just straightforwardly mandate, e.g. abstract method or similar, that the |
|
Ok, I think what I'd like to do is get the conflicts resolved, then merge this and open a couple of issues for any post-hoc cleanup. |
Codecov Report
@@ Coverage Diff @@
## master #928 +/- ##
==========================================
+ Coverage 93.38% 93.88% +0.49%
==========================================
Files 138 140 +2
Lines 6911 6902 -9
Branches 699 693 -6
==========================================
+ Hits 6454 6480 +26
+ Misses 332 311 -21
+ Partials 125 111 -14
Continue to review full report at Codecov.
|
Hmm. The I suppose we could make |
Closes #926
I have:
Description
The aim of this PR is to encapsulate all parameters and logic handling related to spatial levels / aggregation units in a set of new classes.
In
flowmachine/core/spatial_unit.pyI have defined the following new classes:SpatialUnitMixin: Provides alocation_id_columnsproperty (which replacesget_columns_for_level) and alocation_subset_clausemethod which returns a SQLWHEREclause to subset a query to one or more locations (used byFirstLocation), along with methods for checking whether a spatial unit has certain criteria (e.g. represents a join to geographic data, or has longitude/latitude columns).CellSpatialUnit: Represents "cell" level (i.e. where no location join is required).GeomSpatialUnit: Inherits fromQuery. This is an abstract base class which represents a query that maps cell location IDs to geographic locations. Has aget_geom_querymethod which returns a SQL query to select thelocation_id_columnsalong with ageomcolumn containing the geometry (this is used byGeoDataMixinandDistanceMatrix, among others).LonLatSpatialUnit: Derived fromGeomSpatialUnit- represents the point-like levels ("lat-lon", "versioned-site", "versioned-cell").PolygonSpatialUnit: Derived fromGeomSpatialUnit- represents the polygon levels ("polygon", "admin*", "grid").spatial_unit.pyalso contains functionsversioned_cell_spatial_unit,versioned_site_spatial_unit,admin_spatial_unit,grid_spatial_unitandmake_spatial_unit, to help in creating spatial unit objects.All classes which previously took a
levelparameter now accept aspatial_unitparameter instead, which should be a spatial unit object.I have removed
cell_mappings.py, as theCellTo*classes are superseded byPolygonSpatialUnit.I've added a
Geographyclass, which wraps the spatial unitget_geom_querymethod as aQueryobject withGeoDataMixin. This class is used by the FlowMachine server "get_geography" action.Gridhas moved fromflowmachine/features/spatialtoflowmachine/coreso that it can be imported inspatial_unit.py.Now that spatial units are themselves
Queryobjects, these have replaced theCustomQuery/CellTo*objects which were created internally inJoinToLocation. I've also added alocation_joined_queryhelper function which returns aJoinToLocationwhen a location join is required, or the original query object when no join is required (i.e. cell spatial unit), to avoid replicating this if/else logic in several other places.I've removed the
_get_location_joinmethod ofGeoDataMixin, which did a recursive search through the dependency tree to find aJoinToLocationobject. Instead, any query which usesGeoDataMixinwithout redefining_geo_augmented_querymust now have aspatial_unitattribute, whoseget_geom_querymethod is used to add a "geom" column.Previously, the
_SubscriberCellsclass represented cell-level subscriber locations, and thesubscriber_locationsfunction returned either a_SubscriberCellsorJoinToLocationobject depending on the level. Now that I've addedlocation_joined_queryto handle this if/else case, I've combined_SubscriberCellsandsubscriber_locationsinto aSubscriberLocationsclass. Similarly, I've combined_TotalCellEventsintoTotalLocationEvents.I've moved the marshmallow
AggregationUnitcustom field out ofquery_schemas/custom_fields.pyintoquery_schemas/aggregation_unit.py, and added a functionget_spatial_unit_objfor getting a spatial unit object from anaggregation_unitstring.A couple of related thoughts which I think fall outside the scope of this PR:
DistanceMatrixto non-point locations. If so, this will hopefully be an easier task after this refactoring. If not, and we wantDistanceMatrixto continue to support only versioned-cell and versioned-site spatial units, it may make more sense to calculate and store a distance matrix permanently in FlowDB.Geographyis currently exposed as a query kind through the FlowAPIrun/poll/getroute, as well asgeography. This simplifies adding permissions for the geography route, but doesn't make a lot of sense for users because thegetroute won't format the response as GeoJSON bere returning it.